Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix screenshots test #5558

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Dec 6, 2023

Fixes the broken screenshots test.


This change is Reviewable

Copy link

linear bot commented Dec 6, 2023

@rablador rablador force-pushed the fix-the-screenshot-automation-ios-405 branch 15 times, most recently from cb6d731 to daaca09 Compare December 6, 2023 14:02
@rablador rablador marked this pull request as draft December 6, 2023 14:21
@rablador rablador force-pushed the fix-the-screenshot-automation-ios-405 branch 13 times, most recently from a290f36 to 4521e6c Compare December 7, 2023 09:39
@rablador rablador added the iOS Issues related to iOS label Dec 7, 2023
@rablador rablador marked this pull request as ready for review December 7, 2023 14:39
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To run the tests locally you can either run them in Xcode or through fastlane. The latter takes actual screenshots and require some additional setup: https://github.com/mullvad/mullvadvpn-app/tree/main/ios#screenshots-for-appstore

Running the tests in Xcode require only this configuration step of above: https://github.com/mullvad/mullvadvpn-app/tree/main/ios#configuration

Reviewable status: 0 of 5 files reviewed, all discussions resolved

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the screenshot tests ran as part of the regular test suite? If not, we should change the regular ios.yml workflow to execute those tests too.

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, 5 unresolved discussions (waiting on @rablador)


.github/workflows/ios-screenshots.yml line 9 at r1 (raw file):

      - ios/.swiftformat
      - ios/**/*.swift
      - ios/**/*.xctestplan

When I tag releases, I often change just a single XCConfig and the Assets/changes.txt files, and since the all the push condiitons need to be satisfied, screenshots would never be ran.


.github/workflows/ios-screenshots.yml line 16 at r1 (raw file):

  check-formatting:
    name: Check formatting
    runs-on: macos-13-xlarge

I don't believe we need to run this job for screenshots.


.github/workflows/ios-screenshots.yml line 33 at r1 (raw file):

  swiftlint:
    name: Run swiftlint

Nor should we run swiftlint here too.


.github/workflows/ios-screenshots.yml line 48 at r1 (raw file):

  test:
    name: Take screenshots
    runs-on: macos-13-xlarge

We should try out different instance sizes and see if a smaller instance would work just as well - this job won't ever be blocking a merge, so it isn't critical that these tests must finish fast.


.github/workflows/ios-screenshots.yml line 99 at r1 (raw file):

          brew install xcbeautify

      - name: Run screenshot tests

A more apt name would be Create screenshots, I think.


ios/Snapfile line 6 at r1 (raw file):

  "iPhone 11 Pro Max", # 6.5"
  "iPhone 14 Pro Max", # 6.7"
  "iPad Pro (12.9-inch) (2nd generation)",

I'll double check this tomorrow, but we might want the older iPad too.

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, if running the screenshot test itself would take far too long, then we can run it on a slower instance once a day instead of doing it for every commit 🤷 .

Reviewable status: 2 of 5 files reviewed, 5 unresolved discussions (waiting on @rablador)

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iOS changes look fine to me

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @rablador)

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @rablador)


ios/Snapfile line 6 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I'll double check this tomorrow, but we might want the older iPad too.

Yep, we need both the 6th gen and 2nd gen 12.9" iPad Pros.

@rablador rablador force-pushed the fix-the-screenshot-automation-ios-405 branch 6 times, most recently from ff012ce to d4317b6 Compare December 8, 2023 12:41
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing with @pinkisemils IRL we decided to run the screenshot tests in the same github action. They add about a minute to the action overall.

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pinkisemils)


.github/workflows/ios-screenshots.yml line 9 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

When I tag releases, I often change just a single XCConfig and the Assets/changes.txt files, and since the all the push condiitons need to be satisfied, screenshots would never be ran.

Done.


.github/workflows/ios-screenshots.yml line 16 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I don't believe we need to run this job for screenshots.

Done.


.github/workflows/ios-screenshots.yml line 33 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Nor should we run swiftlint here too.

Done.


.github/workflows/ios-screenshots.yml line 99 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

A more apt name would be Create screenshots, I think.

Done


ios/Snapfile line 6 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Yep, we need both the 6th gen and 2nd gen 12.9" iPad Pros.

There are the ones we can get from github:

iPad Air (5th generation)
iPad (10th generation)
iPad mini (6th generation)
iPad Pro (11-inch) (4th generation)
iPad Pro (12.9-inch) (6th generation)

After IRL discussion we settled on adding iPad Pro (11-inch) (4th generation) as well.

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


.github/workflows/ios-screenshots.yml line 48 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

We should try out different instance sizes and see if a smaller instance would work just as well - this job won't ever be blocking a merge, so it isn't critical that these tests must finish fast.

I tried running the regular tests on the slower intel mac, and it's slower than it is cheap, so it'd cost about the same.

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this :)

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rablador rablador force-pushed the fix-the-screenshot-automation-ios-405 branch 3 times, most recently from de0e52c to bf0182f Compare December 8, 2023 14:25
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @pinkisemils)


.github/workflows/ios-screenshots.yml line 48 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I tried running the regular tests on the slower intel mac, and it's slower than it is cheap, so it'd cost about the same.

Running fastlane on the older runner doesn't work. Discussed IRL, so I'll set this discussion to resolved.

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the fix-the-screenshot-automation-ios-405 branch from bf0182f to a4d678b Compare December 8, 2023 14:52
@buggmagnet buggmagnet merged commit 6873304 into main Dec 8, 2023
6 checks passed
@buggmagnet buggmagnet deleted the fix-the-screenshot-automation-ios-405 branch December 8, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants